packages/loot-core/src/server/accounts/sync.ts (4)

355-356: Skipping non-cleared transactions
If importPending is false, non-cleared transactions are skipped altogether. Consider how these pending transactions might be handled if the user later enables pending imports.

---

369-369: Confirm the strict requirement for a 'date'
Throwing an error if date is missing halts the entire sync. Consider skipping or partially applying such transactions instead of stopping the process.

---

377-377: Ensure empty strings are replaced
Using || payeeName won't replace trans.imported_payee if it's an empty string. A trim-based fallback may help unify the logic.

---

389-389: Assess storing raw synced data
raw_synced_data may include personal or sensitive data. Consider encrypting it, partially redacting it, or limiting its size to avoid potential data exposure or excessive bloat.
[security]

Also applies to: 394-394, 395-395, 400-400

packages/loot-core/src/types/models/account.d.ts (1)

21-21: Consider using a typed date
If last_sync is meant for date operations, storing it as a string may limit type-safety. Evaluate using an ISO datetime or a dedicated date type.

packages/loot-core/src/types/models/transaction.d.ts (1)

35-35: Simplify the type definition.

The type definition can be simplified since the undefined type is redundant for optional properties.

-  raw_synced_data?: string | undefined;
+  raw_synced_data?: string;

packages/loot-core/src/server/util/custom-sync-mapping.ts (1)

21-38: Refactor default mappings to reduce duplication.

The mappings for 'payment' and 'deposit' are identical. Consider extracting the common mappings to reduce duplication.

+const commonFieldMappings = new Map([
+  ['date', 'date'],
+  ['payee', 'payeeName'],
+  ['notes', 'remittanceInformationUnstructured'],
+]);
+
 export const defaultMappings: Mappings = new Map([
-  [
-    'payment',
-    new Map([
-      ['date', 'date'],
-      ['payee', 'payeeName'],
-      ['notes', 'remittanceInformationUnstructured'],
-    ]),
-  ],
-  [
-    'deposit',
-    new Map([
-      ['date', 'date'],
-      ['payee', 'payeeName'],
-      ['notes', 'remittanceInformationUnstructured'],
-    ]),
-  ],
+  ['payment', new Map(commonFieldMappings)],
+  ['deposit', new Map(commonFieldMappings)],
 ]);

packages/desktop-client/src/components/banksync/AccountsHeader.tsx (2)

14-14: Remove empty style object.

The empty style object on TableHeader is unnecessary.

-    <TableHeader style={{}}>
+    <TableHeader>

---

15-34: Enhance accessibility and reduce style duplication.

The table header cells need aria labels for better accessibility. Also, the padding style is duplicated across cells.

+const cellStyle = { paddingLeft: '10px' };
+
 <Cell
   value={t('Account')}
   width={!unlinked ? 250 : 'flex'}
-  style={{ paddingLeft: '10px' }}
+  style={cellStyle}
+  aria-label={t('Account column header')}
 />
 {!unlinked && (
   <>
     <Cell
       value={t('Bank')}
       width="flex"
-      style={{ paddingLeft: '10px' }}
+      style={cellStyle}
+      aria-label={t('Bank column header')}
     />
     <Cell
       value={t('Last sync')}
       width={160}
-      style={{ paddingLeft: '10px' }}
+      style={cellStyle}
+      aria-label={t('Last sync column header')}
     />
-    <Cell value="" width={100} style={{ paddingLeft: '10px' }} />
+    <Cell value="" width={100} style={cellStyle} />
   </>
 )}

packages/desktop-client/src/components/banksync/AccountsList.tsx (2)

9-14: Use literal union type for action.

The action type can be more strictly typed using a literal union type.

+type AccountAction = 'link' | 'edit';
+
 type AccountsListProps = {
   accounts: AccountEntity[];
   hoveredAccount?: string | undefined;
   onHover: (id: AccountEntity['id']) => void;
-  onAction: (account: AccountEntity, action: 'link' | 'edit') => void;
+  onAction: (account: AccountEntity, action: AccountAction) => void;
 };

---

28-40: Optimize performance for large lists.

For better performance with large account lists, consider memoizing the AccountRow components and callback functions.

+import React, { useCallback, useMemo } from 'react';
+
+const MemoizedAccountRow = React.memo(AccountRow);
+
 export function AccountsList({
   accounts,
   hoveredAccount,
   onHover,
   onAction,
 }: AccountsListProps) {
+  const handleHover = useCallback(onHover, [onHover]);
+  const handleAction = useCallback(onAction, [onAction]);
+
   if (accounts.length === 0) {
     return null;
   }
+
+  const accountRows = useMemo(
+    () =>
+      accounts.map(account => {
+        const hovered = hoveredAccount === account.id;
+        return (
+          <MemoizedAccountRow
+            key={account.id}
+            account={account}
+            hovered={hovered}
+            onHover={handleHover}
+            onAction={handleAction}
+          />
+        );
+      }),
+    [accounts, hoveredAccount, handleHover, handleAction],
+  );

   return (
     <View>
-      {accounts.map(account => {
-        const hovered = hoveredAccount === account.id;
-
-        return (
-          <AccountRow
-            key={account.id}
-            account={account}
-            hovered={hovered}
-            onHover={onHover}
-            onAction={onAction}
-          />
-        );
-      })}
+      {accountRows}
     </View>
   );

packages/loot-core/src/types/prefs.d.ts (1)

31-33: LGTM! Consider adding JSDoc comments.

The new preference types follow the existing pattern and naming conventions. To improve maintainability, consider adding JSDoc comments explaining the purpose of each preference type.

+  /** Custom field mappings for bank sync providers */
   | `custom-sync-mappings-${string}`
+  /** Controls import of pending transactions per account */
   | `sync-import-pending-${string}`
+  /** Controls import of transaction notes per account */
   | `sync-import-notes-${string}`

packages/desktop-client/src/components/banksync/AccountRow.tsx (2)

13-20: Consider moving tsToString to a shared utility module.

This timestamp formatting utility could be useful in other components. Consider moving it to a shared utility module for reusability.

---

47-48: Use optional chaining for event handlers.

The static analysis tool correctly identifies potential null reference issues. Use optional chaining for cleaner code.

-        onMouseEnter={() => onHover && onHover(account.id)}
-        onMouseLeave={() => onHover && onHover(null)}
+        onMouseEnter={() => onHover?.(account.id)}
+        onMouseLeave={() => onHover?.(null)}

🧰 Tools

🪛 Biome (1.9.4)

[error] 47-47: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

---

[error] 48-48: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

packages/desktop-client/src/components/banksync/index.tsx (3)

24-34: Consider making provider list more maintainable.

The provider names are currently hardcoded in the hook. Consider moving these to a configuration file or deriving them from the BankSyncProviders type to make future additions easier.

+import { type BankSyncProviders } from 'loot-core/types/models';
+
+const SYNC_PROVIDERS: Record<BankSyncProviders, string> = {
+  goCardless: 'GoCardless',
+  simpleFin: 'SimpleFIN',
+};
+
 const useSyncSourceReadable = () => {
   const { t } = useTranslation();
 
   const syncSourceReadable: Record<SyncProviders, string> = {
-    goCardless: 'GoCardless',
-    simpleFin: 'SimpleFIN',
+    ...SYNC_PROVIDERS,
     unlinked: t('Unlinked'),
   };

---

48-74: Optimize memoization of grouped accounts.

The groupedAccounts memo could be optimized by extracting the reducer function and memoizing it separately.

+const useAccountGrouping = () => {
+  const groupAccounts = useCallback((accounts: AccountEntity[]) => {
+    return accounts
+      .filter(a => !a.closed)
+      .reduce(
+        (acc, a) => {
+          const syncSource = a.account_sync_source ?? 'unlinked';
+          acc[syncSource] = acc[syncSource] || [];
+          acc[syncSource].push(a);
+          return acc;
+        },
+        {} as Record<SyncProviders, AccountEntity[]>,
+      );
+  }, []);
+
+  return { groupAccounts };
+};

 export function BankSync() {
+  const { groupAccounts } = useAccountGrouping();
   const groupedAccounts = useMemo(() => {
-    const unsorted = accounts
-      .filter(a => !a.closed)
-      .reduce(
-        (acc, a) => {
-          const syncSource = a.account_sync_source ?? 'unlinked';
-          acc[syncSource] = acc[syncSource] || [];
-          acc[syncSource].push(a);
-          return acc;
-        },
-        {} as Record<SyncProviders, AccountEntity[]>,
-      );
+    const unsorted = groupAccounts(accounts);

---

93-95: Memoize the onHover callback.

The onHover callback should be memoized to prevent unnecessary re-renders of child components.

-  const onHover = useCallback((id: AccountEntity['id']) => {
+  const onHover = useCallback((id: AccountEntity['id']) => {
     setHoveredAccount(id);
-  }, []);
+  }, [setHoveredAccount]);

packages/desktop-client/src/components/banksync/FieldMapping.tsx (1)

16-31: Extract transaction direction options to constants.

Consider moving the transaction direction options to a separate constants file for better maintainability.

+const TRANSACTION_DIRECTIONS = {
+  PAYMENT: 'payment',
+  DEPOSIT: 'deposit',
+} as const;
+
+type TransactionDirectionType = typeof TRANSACTION_DIRECTIONS[keyof typeof TRANSACTION_DIRECTIONS];
+
 const useTransactionDirectionOptions = () => {
   const { t } = useTranslation();
 
   const transactionDirectionOptions = [
     {
-      value: 'payment',
+      value: TRANSACTION_DIRECTIONS.PAYMENT,
       label: t('Payment'),
     },
     {
-      value: 'deposit',
+      value: TRANSACTION_DIRECTIONS.DEPOSIT,
       label: t('Deposit'),
     },
   ];

packages/desktop-client/src/components/banksync/EditSyncAccount.tsx (2)

42-66: Extract mappable fields configuration.

Consider moving the mappable fields configuration to a separate file for better maintainability and reusability.

+// mappableFieldsConfig.ts
+export const MAPPABLE_FIELDS_CONFIG: MappableField[] = [
+  {
+    actualField: 'date',
+    syncFields: ['date', 'bookingDate', 'valueDate'],
+  },
+  // ... other fields
+];

-const mappableFields: MappableField[] = [
-  {
-    actualField: 'date',
-    syncFields: ['date', 'bookingDate', 'valueDate'],
-  },
-  // ... rest of the fields
-];
+import { MAPPABLE_FIELDS_CONFIG } from './mappableFieldsConfig';
+
+const mappableFields = MAPPABLE_FIELDS_CONFIG;

---

86-93: Extract preference management to custom hook.

Consider creating a custom hook to manage related preferences, improving code organization and reusability.

+const useAccountPreferences = (accountId: string) => {
+  const [savedMappings, setSavedMappings] = useSyncedPref(
+    `custom-sync-mappings-${accountId}`,
+    mappingsToString(defaultMappings)
+  );
+  const [savedImportNotes, setSavedImportNotes] = useSyncedPref(
+    `sync-import-notes-${accountId}`,
+    true
+  );
+  const [savedImportPending, setSavedImportPending] = useSyncedPref(
+    `sync-import-pending-${accountId}`,
+    true
+  );
+
+  return {
+    savedMappings,
+    setSavedMappings,
+    savedImportNotes,
+    setSavedImportNotes,
+    savedImportPending,
+    setSavedImportPending,
+  };
+};

packages/loot-core/src/server/main.ts (1)

1064-1066: Consider using ISO string for better timestamp readability.

While storing milliseconds since epoch works, using an ISO string format would make the timestamps more readable in the database while maintaining sortability.

-const ts = new Date().getTime().toString();
+const ts = new Date().toISOString();

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

Learnt from: matt-fidd
PR: actualbudget/actual#3581
File: packages/loot-core/src/types/models/bank-sync.d.ts:11-21
Timestamp: 2024-11-10T16:45:25.627Z
Learning: In `packages/loot-core/src/types/models/bank-sync.d.ts`, when defining TypeScript types for data received from the server, maintain the field names as they are in the server response, even if they don't follow TypeScript naming conventions.

Learnt from: matt-fidd
PR: actualbudget/actual#3581
File: packages/loot-core/src/types/models/bank-sync.d.ts:11-21
Timestamp: 2024-11-10T16:45:25.627Z
Learning: In `packages/loot-core/src/types/models/bank-sync.d.ts`, when defining TypeScript types for data received from the server, maintain the field names as they are in the server response, even if they don't follow TypeScript naming conventions.

Learnt from: matt-fidd
PR: actualbudget/actual#4141
File: packages/loot-core/src/server/aql/schema/index.ts:145-145
Timestamp: 2025-01-12T20:26:15.249Z
Learning: In the schema definitions in `packages/loot-core/src/server/aql/schema/index.ts`, field definitions should follow the existing pattern using only the common properties (`type`, `default`, `required`, `ref`) for consistency.

Learnt from: joel-jeremy
PR: actualbudget/actual#3685
File: packages/desktop-client/src/components/accounts/Account.tsx:655-665
Timestamp: 2024-11-10T16:45:31.225Z
Learning: The Account component in 'packages/desktop-client/src/components/accounts/Account.tsx' is being rewritten in a separate PR.

Learnt from: joel-jeremy
PR: actualbudget/actual#3685
File: packages/desktop-client/src/components/accounts/Account.tsx:655-665
Timestamp: 2024-11-10T16:45:31.225Z
Learning: The Account component in 'packages/desktop-client/src/components/accounts/Account.tsx' is being rewritten in a separate PR.

Learnt from: joel-jeremy
PR: actualbudget/actual#3685
File: packages/desktop-client/src/components/accounts/Account.tsx:655-665
Timestamp: 2024-11-10T16:45:31.225Z
Learning: The Account component in 'packages/desktop-client/src/components/accounts/Account.tsx' is being rewritten in a separate PR.

packages/desktop-client/src/components/banksync/index.tsx (2)

1-23: Consider code splitting or lazy-loading for improved performance.

The comprehensive import list is fine, but if the bundle grows significantly, you may consider code-splitting or lazy-loading less frequently used components, especially for modals or config data, to optimize performance.

---

24-34: Localize all sync provider strings.

While "Unlinked" is localized via t(...), strings like "GoCardless" and "SimpleFIN" are currently hardcoded. Consider wrapping them in t(...) for consistent localization support.

-    goCardless: 'GoCardless',
-    simpleFin: 'SimpleFIN',
+    goCardless: t('GoCardless'),
+    simpleFin: t('SimpleFIN'),

packages/desktop-client/src/components/banksync/AccountsHeader.tsx (2)

14-14: Remove unnecessary empty style object.

The empty style object style={{}} on TableHeader serves no purpose and can be removed.

-    <TableHeader style={{}}>
+    <TableHeader>

---

15-33: Consider extracting hardcoded values to constants.

The component has multiple instances of hardcoded padding and width values. Consider extracting these to theme constants or a configuration object for better maintainability and consistency.

+const CELL_STYLES = {
+  padding: { paddingLeft: '10px' },
+  widths: {
+    account: 250,
+    lastSync: 160,
+    actions: 100,
+  },
+} as const;

 export function AccountsHeader({ unlinked }: AccountsHeaderProps) {
   const { t } = useTranslation();

   return (
     <TableHeader>
       <Cell
         value={t('Account')}
-        width={!unlinked ? 250 : 'flex'}
+        width={!unlinked ? CELL_STYLES.widths.account : 'flex'}
-        style={{ paddingLeft: '10px' }}
+        style={CELL_STYLES.padding}
       />
       {!unlinked && (
         <>
           <Cell
             value={t('Bank')}
             width="flex"
-            style={{ paddingLeft: '10px' }}
+            style={CELL_STYLES.padding}
           />
           <Cell
             value={t('Last sync')}
-            width={160}
-            style={{ paddingLeft: '10px' }}
+            width={CELL_STYLES.widths.lastSync}
+            style={CELL_STYLES.padding}
           />
-          <Cell value="" width={100} style={{ paddingLeft: '10px' }} />
+          <Cell value="" width={CELL_STYLES.widths.actions} style={CELL_STYLES.padding} />
         </>
       )}
     </TableHeader>
   );
 }

packages/desktop-client/src/components/banksync/AccountsList.tsx (1)

16-43: Consider memoizing callbacks and mapped components for performance.

While the implementation is clean, consider these performance optimizations:

1. Memoize the onHover and onAction callbacks using useCallback
2. Memoize the mapped AccountRow components using useMemo

+import React, { useCallback, useMemo } from 'react';
 
 export function AccountsList({
   accounts,
   hoveredAccount,
   onHover,
   onAction,
 }: AccountsListProps) {
   if (accounts.length === 0) {
     return null;
   }
 
+  const memoizedOnHover = useCallback(onHover, [onHover]);
+  const memoizedOnAction = useCallback(onAction, [onAction]);
+
+  const accountRows = useMemo(
+    () =>
+      accounts.map(account => {
+        const hovered = hoveredAccount === account.id;
+
+        return (
+          <AccountRow
+            key={account.id}
+            account={account}
+            hovered={hovered}
+            onHover={memoizedOnHover}
+            onAction={memoizedOnAction}
+          />
+        );
+      }),
+    [accounts, hoveredAccount, memoizedOnHover, memoizedOnAction]
+  );
 
   return (
     <View>
-      {accounts.map(account => {
-        const hovered = hoveredAccount === account.id;
-
-        return (
-          <AccountRow
-            key={account.id}
-            account={account}
-            hovered={hovered}
-            onHover={onHover}
-            onAction={onAction}
-          />
-        );
-      })}
+      {accountRows}
     </View>
   );
 }

packages/loot-core/src/server/util/custom-sync-mapping.ts (2)

1-29: Enhance type safety and error handling.

While the implementation is good, consider these improvements:

1. Add type guards for better type safety
2. Add validation for the inner map values
3. Make error messages more specific

+type JsonMapping = Record<string, Record<string, string>>;
+
+function isValidInnerMapping(value: unknown): value is Record<string, string> {
+  return (
+    typeof value === 'object' &&
+    value !== null &&
+    Object.entries(value).every(
+      ([k, v]) => typeof k === 'string' && typeof v === 'string'
+    )
+  );
+}
+
 export const mappingsFromString = (str: string): Mappings => {
   try {
     const parsed = JSON.parse(str);
     if (typeof parsed !== 'object' || parsed === null) {
-      throw new Error('Invalid mapping format');
+      throw new Error('Mapping must be a JSON object');
     }
+    
+    for (const [key, value] of Object.entries(parsed)) {
+      if (!isValidInnerMapping(value)) {
+        throw new Error(
+          `Invalid inner mapping for key "${key}": all values must be strings`
+        );
+      }
+    }
+
     return new Map(
-      Object.entries(parsed).map(([key, value]) => [
+      Object.entries(parsed as JsonMapping).map(([key, value]) => [
         key,
         new Map(Object.entries(value as object)),
       ]),
     );
   } catch (e) {
     const message = e instanceof Error ? e.message : e;
-    throw new Error(`Failed to parse mapping: ${message}`);
+    throw new Error(
+      e instanceof SyntaxError
+        ? 'Invalid JSON format in mapping string'
+        : `Failed to parse mapping: ${message}`
+    );
   }
 };

---

31-48: Reduce duplication in default mappings.

The mappings for 'payment' and 'deposit' are identical. Consider extracting the common mapping to reduce duplication.

+const DEFAULT_FIELD_MAPPING = new Map([
+  ['date', 'date'],
+  ['payee', 'payeeName'],
+  ['notes', 'remittanceInformationUnstructured'],
+]);
+
 export const defaultMappings: Mappings = new Map([
-  [
-    'payment',
-    new Map([
-      ['date', 'date'],
-      ['payee', 'payeeName'],
-      ['notes', 'remittanceInformationUnstructured'],
-    ]),
-  ],
-  [
-    'deposit',
-    new Map([
-      ['date', 'date'],
-      ['payee', 'payeeName'],
-      ['notes', 'remittanceInformationUnstructured'],
-    ]),
-  ],
+  ['payment', new Map(DEFAULT_FIELD_MAPPING)],
+  ['deposit', new Map(DEFAULT_FIELD_MAPPING)],
 ]);

packages/desktop-client/src/components/banksync/AccountRow.tsx (3)

13-18: Consider moving tsToString to a shared utility.

The tsToString function could be useful in other components that need to format timestamps. Consider moving it to a shared utility file.

---

45-46: Use optional chaining for event handlers.

The onHover callback checks could be simplified using optional chaining.

-        onMouseEnter={() => onHover && onHover(account.id)}
-        onMouseLeave={() => onHover && onHover(null)}
+        onMouseEnter={() => onHover?.(account.id)}
+        onMouseLeave={() => onHover?.(null)}

🧰 Tools

🪛 Biome (1.9.4)

[error] 45-45: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

---

[error] 46-46: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

---

75-87: Consider extracting button cell to a separate component.

The conditional rendering of edit/link buttons could be extracted to a separate component to improve readability and reusability.

type ActionButtonProps = {
  account: AccountEntity;
  onAction: AccountRowProps['onAction'];
};

const ActionButton: React.FC<ActionButtonProps> = ({ account, onAction }) => {
  if (account.account_sync_source) {
    return (
      <Cell name="edit" plain style={{ paddingRight: '10px' }}>
        <Button onPress={() => onAction(account, 'edit')}>
          <Trans>Edit</Trans>
        </Button>
      </Cell>
    );
  }
  return (
    <Cell name="link" plain style={{ paddingRight: '10px' }}>
      <Button onPress={() => onAction(account, 'link')}>
        <Trans>Link account</Trans>
      </Button>
    </Cell>
  );
};

packages/desktop-client/src/components/banksync/FieldMapping.tsx (2)

16-31: Consider moving hook to a separate file.

The useTransactionDirectionOptions hook could be moved to a separate hooks file for better organization and reusability.

---

85-152: Extract field row to a separate component.

The field mapping row rendering logic is complex and could be extracted to a separate component for better maintainability.

type FieldRowProps = {
  field: MappableFieldWithExample;
  mapping: Map<string, string>;
  setMapping: (field: string, value: string) => void;
};

const FieldRow: React.FC<FieldRowProps> = ({ field, mapping, setMapping }) => {
  const { t } = useTranslation();
  
  return (
    <Row
      key={field.actualField}
      style={{
        fontSize: 13,
        backgroundColor: theme.tableRowBackgroundHover,
        display: 'flex',
        alignItems: 'center',
        border: '1px solid var(--color-tableBorder)',
        minHeight: '40px',
      }}
      collapsed={true}
    >
      {/* ... rest of the row content ... */}
    </Row>
  );
};

packages/desktop-client/src/components/banksync/EditSyncAccount.tsx (2)

42-66: Add documentation for sync fields.

Consider adding JSDoc comments to explain the purpose and expected format of each sync field, especially for fields like remittanceInformationUnstructured and remittanceInformationStructured.

---

135-141: Add loading state for preference updates.

The onSave function updates multiple preferences but doesn't handle the loading state or potential failures.

Consider this improvement:

   const onSave = async (close: () => void) => {
+    try {
+      setIsSaving(true);
       const mappingsStr = mappingsToString(mappings);
-      setSavedMappings(mappingsStr);
-      setSavedImportPending(String(importPending));
-      setSavedImportNotes(String(importNotes));
+      await Promise.all([
+        setSavedMappings(mappingsStr),
+        setSavedImportPending(String(importPending)),
+        setSavedImportNotes(String(importNotes))
+      ]);
       close();
+    } catch (error) {
+      console.error('Failed to save preferences:', error);
+      // Show error notification to user
+    } finally {
+      setIsSaving(false);
+    }
   };

packages/loot-core/src/server/aql/schema/index.ts (1)

56-57: Add documentation for new schema fields.

Consider adding comments to explain:

• The purpose and format of raw_synced_data in transactions
• The format and timezone of last_sync in accounts

Also applies to: 77-77

packages/loot-core/migrations/1738072673000_bank_sync_page.sql (1)

1-7: Consider enhancing the schema with constraints and indexes.

While the migration is functionally correct, consider these improvements:

1. Add a default value (NULL) for new columns
2. Add an index on last_sync if it will be frequently queried
3. Consider adding CHECK constraints if there are valid format requirements for these fields

BEGIN TRANSACTION;

-ALTER TABLE accounts ADD COLUMN last_sync text;
+ALTER TABLE accounts ADD COLUMN last_sync text DEFAULT NULL;
+CREATE INDEX idx_accounts_last_sync ON accounts(last_sync);

-ALTER TABLE transactions ADD COLUMN raw_synced_data text;
+ALTER TABLE transactions ADD COLUMN raw_synced_data text DEFAULT NULL;

COMMIT;

packages/loot-core/src/types/models/account.d.ts (1)

13-14: Maintain consistent field naming convention.

The field naming is inconsistent:

• bankName and bankId use camelCase
• last_sync uses snake_case

Based on the retrieved learning, we should maintain consistency with server field names. Additionally, consider adding JSDoc comments to document the purpose of these fields.

-  bankName: T extends true ? string : null;
-  bankId: T extends true ? number : null;
+  /** Name of the bank as provided by the sync provider */
+  bank_name: T extends true ? string : null;
+  /** Unique identifier for the bank in the provider's system */
+  bank_id: T extends true ? number : null;
   mask: T extends true ? string : null; // end of bank account number
   official_name: T extends true ? string : null;
   balance_current: T extends true ? number : null;
   balance_available: T extends true ? number : null;
   balance_limit: T extends true ? number : null;
   account_sync_source: T extends true ? AccountSyncSource : null;
+  /** Timestamp of the last successful sync operation */
   last_sync: T extends true ? string : null;

Also applies to: 21-21

packages/loot-core/src/client/accounts/accountsSlice.ts (1)

193-194: Consider optimizing the account refresh.

While refreshing accounts after sync is necessary, consider debouncing multiple sync responses to prevent unnecessary re-renders when batch syncing accounts.

+import { debounce } from '../../shared/util';
+
+const debouncedGetAccounts = debounce((dispatch) => {
+  dispatch(getAccounts());
+}, 1000);
+
 function handleSyncResponse(
   accountId: AccountEntity['id'],
   res: SyncResponse,
   dispatch: AppDispatch,
   resNewTransactions: Array<TransactionEntity['id']>,
   resMatchedTransactions: Array<TransactionEntity['id']>,
   resUpdatedAccounts: Array<AccountEntity['id']>,
 ) {
   // ... existing code ...
-  dispatch(getAccounts());
+  debouncedGetAccounts(dispatch);

packages/loot-core/src/server/accounts/sync.ts (2)

353-355: Consider adding a preference for cleared status.

Currently, the code hardcodes the cleared status based on booked. Consider making this configurable like importPending and importNotes.

---

394-395: Sanitize notes before storage.

The code replaces '#' with '##' but should consider other potential markdown characters that might need escaping.

-        notes: importNotes ? notes.trim().replace('#', '##') : null,
+        notes: importNotes && notes ? sanitizeMarkdown(notes.trim()) : null,

Consider adding a utility function:

function sanitizeMarkdown(text: string): string {
  return text
    .replace(/#/g, '##')
    .replace(/\*/g, '\\*')
    .replace(/_/g, '\\_');
}

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

Learnt from: matt-fidd
PR: actualbudget/actual#3581
File: packages/loot-core/src/types/models/bank-sync.d.ts:11-21
Timestamp: 2024-11-10T16:45:25.627Z
Learning: In `packages/loot-core/src/types/models/bank-sync.d.ts`, when defining TypeScript types for data received from the server, maintain the field names as they are in the server response, even if they don't follow TypeScript naming conventions.

Learnt from: MatissJanis
PR: actualbudget/actual#3458
File: packages/loot-core/src/client/state-types/prefs.d.ts:5-5
Timestamp: 2024-11-10T16:45:25.627Z
Learning: In future reviews, ensure that changes related to `PrefsState` in `prefs.d.ts` do not incorrectly assume necessary updates in other parts of the codebase. Verify the impact thoroughly before making suggestions.

Learnt from: matt-fidd
PR: actualbudget/actual#4253
File: packages/desktop-client/src/components/banksync/AccountRow.tsx:0-0
Timestamp: 2025-01-28T23:24:32.957Z
Learning: In the Actual codebase, timestamp formatting should use date-fns' `format` function with explicit format strings (e.g., 'HH:mm:ss' for time) instead of JavaScript's built-in `toLocaleTimeString()` for consistency.

Learnt from: joel-jeremy
PR: actualbudget/actual#3685
File: packages/desktop-client/src/components/accounts/Account.tsx:655-665
Timestamp: 2024-11-10T16:45:31.225Z
Learning: The Account component in 'packages/desktop-client/src/components/accounts/Account.tsx' is being rewritten in a separate PR.

Learnt from: joel-jeremy
PR: actualbudget/actual#3685
File: packages/desktop-client/src/components/accounts/Account.tsx:655-665
Timestamp: 2024-11-10T16:45:31.225Z
Learning: The Account component in 'packages/desktop-client/src/components/accounts/Account.tsx' is being rewritten in a separate PR.

Learnt from: joel-jeremy
PR: actualbudget/actual#3685
File: packages/desktop-client/src/components/accounts/Account.tsx:655-665
Timestamp: 2024-11-10T16:45:31.225Z
Learning: The Account component in 'packages/desktop-client/src/components/accounts/Account.tsx' is being rewritten in a separate PR.

Learnt from: matt-fidd
PR: actualbudget/actual#3581
File: packages/loot-core/src/types/models/bank-sync.d.ts:11-21
Timestamp: 2024-11-10T16:45:25.627Z
Learning: In `packages/loot-core/src/types/models/bank-sync.d.ts`, when defining TypeScript types for data received from the server, maintain the field names as they are in the server response, even if they don't follow TypeScript naming conventions.

packages/loot-core/src/server/main.ts (1)

Line range hint 1048-1066: Add TypeScript types and improve error handling.

The function implementation could be improved in several areas:

1. Add TypeScript types for parameters to improve type safety
2. Add error handling for the database update
3. Consider storing the timestamp as an INTEGER in the database for better sorting and querying
4. Consider returning the modified arrays instead of mutating external state

-async function handleSyncResponse(
-  res,
-  acct,
-  newTransactions,
-  matchedTransactions,
-  updatedAccounts,
-) {
+async function handleSyncResponse(
+  res: { added: Transaction[]; updated: Transaction[] },
+  acct: Account,
+  newTransactions: Transaction[],
+  matchedTransactions: Transaction[],
+  updatedAccounts: string[],
+): Promise<void> {
   const { added, updated } = res;

   newTransactions.push(...added);
   matchedTransactions.push(...updated);

   if (added.length > 0) {
     updatedAccounts.push(acct.id);
   }

   const ts = new Date().getTime().toString();
   const id = acct.id;
-  await db.runQuery(`UPDATE accounts SET last_sync = ? WHERE id = ?`, [ts, id]);
+  try {
+    await db.runQuery(
+      `UPDATE accounts SET last_sync = ? WHERE id = ?`,
+      [ts, id]
+    );
+  } catch (error) {
+    logger.error('Failed to update last_sync timestamp:', error);
+    throw new Error('Failed to update sync timestamp');
+  }
 }

packages/desktop-client/src/components/banksync/EditSyncAccount.tsx (1)

Line range hint 394-395: Add null check for notes trimming.

The notes.trim() call could throw if notes is null or undefined.

Apply this diff to add proper null checking:

-        notes: importNotes ? notes.trim().replace('#', '##') : null,
+        notes: importNotes && notes ? notes.trim().replace('#', '##') : null,

packages/desktop-client/src/components/banksync/EditSyncAccount.tsx (1)

173-179: ⚠️ Potential issue

Remove unsafe non-null assertion.

The non-null assertion on mapping! could lead to runtime errors if mapping is undefined.

Apply this diff to improve type safety:

-              mapping={mapping!}
+              mapping={mapping ?? new Map()}

packages/loot-core/src/server/accounts/sync.ts (1)

361-366: ⚠️ Potential issue

Add validation for mapped fields.

The code should validate that the mapped fields exist in the transaction object before accessing them.

Apply this diff to add proper validation:

-    const mapping = mappings.get(trans.amount <= 0 ? 'payment' : 'deposit');
-
-    const date = trans[mapping.get('date')] ?? trans.date;
-    const payeeName = trans[mapping.get('payee')];
-    const notes = trans[mapping.get('notes')];
+    const direction = trans.amount <= 0 ? 'payment' : 'deposit';
+    const mapping = mappings.get(direction);
+    
+    if (!mapping) {
+      throw new Error(`No mapping found for direction: ${direction}`);
+    }
+    
+    const dateField = mapping.get('date');
+    const payeeField = mapping.get('payee');
+    const notesField = mapping.get('notes');
+    
+    if (!dateField || !payeeField) {
+      throw new Error('Required mapping fields (date, payee) are missing');
+    }
+    
+    const date = trans[dateField] ?? trans.date;
+    const payeeName = trans[payeeField];
+    const notes = notesField ? trans[notesField] : null;

packages/desktop-client/e2e/page-models/bank-sync-page.js (1)

1-10: Consider converting to TypeScript for better type safety.

The page model would benefit from TypeScript conversion to match other files in the codebase and provide better type safety for the page and options parameters.

-export class BankSyncPage {
+export class BankSyncPage {
+  private page: Page;
+  private bankSync: Locator;
+
   constructor(page) {
     this.page = page;
     this.bankSync = page.getByTestId('banksync');
   }

-  async waitFor(options) {
+  async waitFor(options?: WaitForOptions): Promise<void> {
     await this.bankSync.waitFor(options);
   }
 }

packages/desktop-client/e2e/page-models/navigation.js (1)

59-70: Extract duplicated menu expansion logic.

The menu expansion logic is duplicated across multiple navigation methods. Consider extracting it into a reusable helper method.

+  private async expandMoreMenuIfNeeded(link: Locator): Promise<void> {
+    if (!(await link.isVisible())) {
+      await this.page.getByRole('button', { name: 'More' }).click();
+    }
+  }

   async goToBankSyncPage() {
     const bankSyncLink = this.page.getByRole('link', { name: 'Bank Sync' });
-    if (!(await bankSyncLink.isVisible())) {
-      await this.page.getByRole('button', { name: 'More' }).click();
-    }
+    await this.expandMoreMenuIfNeeded(bankSyncLink);
     await bankSyncLink.click();
     return new BankSyncPage(this.page);
   }

packages/desktop-client/src/components/banksync/AccountRow.tsx (4)

12-17: Move timestamp formatting to a shared utility.

The tsToString function could be useful in other components. Consider moving it to a shared utility file.

// New file: src/utils/dateFormatting.ts
+import { format } from 'loot-core/src/shared/months';
+
+export const formatTimestamp = (ts: string | null, dateFormat: string): string => {
+  if (!ts) return 'Unknown';
+  const parsed = new Date(parseInt(ts, 10));
+  return `${format(parsed, dateFormat)} ${format(parsed, 'HH:mm:ss')}`;
+};

---

44-45: Use optional chaining for event handlers.

Replace the conditional checks with optional chaining for cleaner code.

-        onMouseEnter={() => onHover && onHover(account.id)}
-        onMouseLeave={() => onHover && onHover(null)}
+        onMouseEnter={() => onHover?.(account.id)}
+        onMouseLeave={() => onHover?.(null)}

🧰 Tools

🪛 Biome (1.9.4)

[error] 44-44: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

---

[error] 45-45: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

---

47-72: Extract common cell styles to reduce duplication.

The cell styling is duplicated across multiple cells. Consider extracting it to a constant or styled component.

+const commonCellStyle = {
+  color: theme.tableText,
+  padding: '10px',
+};

 <Cell
   name="accountName"
   width={250}
   plain
-  style={{ color: theme.tableText, padding: '10px' }}
+  style={commonCellStyle}
 >

---

74-86: Add aria-label to improve accessibility.

The Edit and Link buttons should have descriptive aria-labels for better accessibility.

-            <Button onPress={() => onAction(account, 'edit')}>
+            <Button 
+              onPress={() => onAction(account, 'edit')}
+              aria-label={`Edit ${account.name} sync settings`}
+            >
               <Trans>Edit</Trans>
             </Button>

-            <Button onPress={() => onAction(account, 'link')}>
+            <Button 
+              onPress={() => onAction(account, 'link')}
+              aria-label={`Link ${account.name} to bank account`}
+            >
               <Trans>Link account</Trans>
             </Button>

packages/desktop-client/src/components/banksync/FieldMapping.tsx (3)

4-5: Consider consolidating icon imports to use a single version.

Icons are imported from two different versions (v0 and v1). For consistency, consider using icons from the same version.

---

16-31: Consider extracting transaction direction options.

The transaction direction options could be defined outside the hook as a constant to improve reusability and reduce re-renders.

+const TRANSACTION_DIRECTIONS = [
+  {
+    value: 'payment',
+    label: 'Payment',
+  },
+  {
+    value: 'deposit',
+    label: 'Deposit',
+  },
+] as const;
+
 const useTransactionDirectionOptions = () => {
   const { t } = useTranslation();
 
-  const transactionDirectionOptions = [
-    {
-      value: 'payment',
-      label: t('Payment'),
-    },
-    {
-      value: 'deposit',
-      label: t('Deposit'),
-    },
-  ];
+  const transactionDirectionOptions = TRANSACTION_DIRECTIONS.map(({ value, label }) => ({
+    value,
+    label: t(label),
+  }));
 
   return { transactionDirectionOptions };
 };

---

85-152: Consider memoizing mapped fields for better performance.

The field mapping operation could be memoized to prevent unnecessary re-renders, especially for large datasets.

+import { useMemo } from 'react';

 export function FieldMapping({
   transactionDirection,
   setTransactionDirection,
   fields,
   mapping,
   setMapping,
 }: FieldMappingProps) {
   const { t } = useTranslation();
   const { transactionDirectionOptions } = useTransactionDirectionOptions();
 
+  const mappedFields = useMemo(
+    () =>
+      fields.map(field => ({
+        key: field.actualField,
+        mappedValue: mapping.get(field.actualField),
+        example: field.syncFields.find(
+          f => f.field === mapping.get(field.actualField),
+        )?.example,
+      })),
+    [fields, mapping],
+  );
 
   return (
     <>
       <Select /* ... */ />
       <TableHeader>/* ... */</TableHeader>
-      {fields.map(field => {
+      {mappedFields.map(({ key, mappedValue, example }) => {
         return (
           <Row
-            key={field.actualField}
+            key={key}
             /* ... */
           >
             /* ... */
           </Row>
         );
       })}
     </>
   );
 }

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

Learnt from: matt-fidd
PR: actualbudget/actual#4253
File: packages/desktop-client/src/components/banksync/AccountRow.tsx:0-0
Timestamp: 2025-01-28T23:24:32.957Z
Learning: In the Actual codebase, timestamp formatting should use date-fns' `format` function with explicit format strings (e.g., 'HH:mm:ss' for time) instead of JavaScript's built-in `toLocaleTimeString()` for consistency.

Learnt from: joel-jeremy
PR: actualbudget/actual#3685
File: packages/desktop-client/src/components/accounts/Account.tsx:655-665
Timestamp: 2024-11-10T16:45:31.225Z
Learning: The Account component in 'packages/desktop-client/src/components/accounts/Account.tsx' is being rewritten in a separate PR.

packages/desktop-client/src/components/banksync/EditSyncAccount.tsx (1)

108-113: Improve boolean state initialization.

The string comparison for boolean state initialization could be simplified using a utility function.

+const toBool = (value: string | boolean) => String(value) === 'true';
+
 const [importPending, setImportPending] = useState(
-  String(savedImportPending) === 'true',
+  toBool(savedImportPending)
 );
 const [importNotes, setImportNotes] = useState(
-  String(savedImportNotes) === 'true',
+  toBool(savedImportNotes)
 );

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro